Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(astro): prerendering issue when path contains underscore #11243

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

V3RON
Copy link
Contributor

@V3RON V3RON commented Jun 12, 2024

function isPublicRoute(file: URL, config: AstroConfig): boolean {
const pagesDir = resolvePages(config);
const parts = file.toString().replace(pagesDir.toString(), '').split('/').slice(1);
for (const part of parts) {
if (part.startsWith('_')) return false;
}
return true;
}

Notice the condition in line 124. It checks for the presence of _ in any part of the URL. The problem is that file is an absolute path, so _GITHUB is picked up by this condition, causing the page to be considered 'not public'.

Changes

Updated isPublicRoute to use relative paths, ensuring correct handling of underscore folders outside the repository. This change ensures the function correctly returns 'true' when an underscore folder is present outside the repository folder, as intended.

Closes #11232

Testing

Added automated test to cover edge cases involving underscores in paths.

Docs

No update needed

Copy link

changeset-bot bot commented Jun 12, 2024

🦋 Changeset detected

Latest commit: 59d11e7

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jun 12, 2024
@V3RON
Copy link
Contributor Author

V3RON commented Jun 12, 2024

Quick note: packages/astro/test/fixtures/_underscore in folder name/node_modules/fake-astro-library/ is there on purpose to simulate case when page is exposed by a third-party library.

@V3RON V3RON force-pushed the fix/underscore-in-paths-prerender branch from 4853821 to cdad175 Compare June 12, 2024 16:10
Comment on lines +127 to +128
// Normalize the file directory path by removing the pagesDir prefix if it exists,
// otherwise remove the rootDir prefix.
Copy link
Member

@ematipico ematipico Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should also explain why this is necessary. The code itself isn't too complicated to understand, so a comment that explains it seems superfluous. However, it's worth adding an explanation of the motive because if you left a comment here, it means there's a particular case to handle :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might happen that a page is located in node_modules, and the Astro project itself is in a path containing the _ character. In such cases, we should use a relative path to bypass the segment containing _.

WDYT?

.changeset/honest-ravens-double.md Outdated Show resolved Hide resolved
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great test, thanks!

@matthewp matthewp merged commit ba2b14c into withastro:main Jun 14, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR behaviour different with hybrid mode and node adapter between Linux and MacOS (ARM)
3 participants